Support server event streams #1479

82marbag · 2022-06-21T17:24:28Z

Support server event streams

Closes: #1157

Motivation and Context

This PR adds support for server event streams. See #1157

Description

This PR adds support for server event streams and updates the Pokemon model and service to use them

Testing

Run the Pokemon server and client
./gradlew :codegen-server-test:test
./gradlew :codegen-test:test

Checklist

I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

jdisanti

This is a great start! It's exciting to see this working 😄

CHANGELOG.next.toml

...tware/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/EventStreamSymbolProvider.kt

...src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/customize/EventStreamDecorator.kt

...in/software/amazon/smithy/rust/codegen/smithy/protocols/HttpBoundProtocolPayloadGenerator.kt

...otlin/software/amazon/smithy/rust/codegen/smithy/transformers/RemoveEventStreamOperations.kt

codegen-server-test/model/pokemon.smithy

...src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/customize/EventStreamDecorator.kt

aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsCodegenDecorator.kt

aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/SigV4SigningDecorator.kt

...in/software/amazon/smithy/rust/codegen/smithy/protocols/HttpBoundProtocolPayloadGenerator.kt

jdisanti

I think you might have misunderstood event stream errors, and that's what most of my comments are about. Other than errors and the removal of the event stream allow list, I think this looks good. Both of these need to be addressed before approval though.

codegen-server-test/model/pokemon.smithy

...er/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ServerCodegenVisitor.kt

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/RustSettings.kt

...tlin/software/amazon/smithy/rust/codegen/smithy/customize/NoOpEventStreamSigningDecorator.kt

...ftware/amazon/smithy/rust/codegen/smithy/protocols/parse/EventStreamUnmarshallerGenerator.kt

...otlin/software/amazon/smithy/rust/codegen/smithy/transformers/RemoveEventStreamOperations.kt

rust-runtime/aws-smithy-eventstream/src/error.rs

rust-runtime/aws-smithy-http/src/event_stream/receiver.rs

rust-runtime/aws-smithy-http/src/event_stream/sender.rs

codegen-server-test/model/pokemon.smithy

...main/kotlin/software/amazon/smithy/rust/codegen/smithy/transformers/EventStreamNormalizer.kt

...kotlin/software/amazon/smithy/rust/codegen/smithy/generators/error/CombinedErrorGenerator.kt

...main/kotlin/software/amazon/smithy/rust/codegen/smithy/transformers/EventStreamNormalizer.kt

...software/amazon/smithy/rust/codegen/server/smithy/generators/ServerCombinedErrorGenerator.kt

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/EventStreamSymbolProvider.kt

...lin/software/amazon/smithy/rust/codegen/smithy/generators/config/EventStreamSigningConfig.kt

...kotlin/software/amazon/smithy/rust/codegen/smithy/generators/error/CombinedErrorGenerator.kt

...kotlin/software/amazon/smithy/rust/codegen/smithy/generators/error/TopLevelErrorGenerator.kt

jdisanti · 2022-07-12T21:28:39Z

...main/kotlin/software/amazon/smithy/rust/codegen/smithy/transformers/EventStreamNormalizer.kt

@@ -37,3 +80,40 @@ object EventStreamNormalizer {
            .build()
    }
 }
+
+fun OperationShape.operationErrors(model: Model): List<Shape> {


What is the intent for this function? The OperationIndex.getErrors already returns all the errors for the operation, right? So why does it need to be filtered by the synthetic trait? Wouldn't all of them have it since the normalizer added it to all of them?

...main/kotlin/software/amazon/smithy/rust/codegen/smithy/transformers/EventStreamNormalizer.kt

rust-runtime/aws-smithy-http/src/event_stream/sender.rs

crisidev

LGTM. I have added some minor comments. This PR will break as soon as you merge in the Python server, so some other adjustments are needed.

One thing I suggest you to do is to throw a CodegenException if EventStreams are used in model built with the Python plugin.

...software/amazon/smithy/rust/codegen/server/smithy/generators/ServerCombinedErrorGenerator.kt

crisidev · 2022-07-13T10:40:51Z

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/RestJson.kt

-        RestJsonHttpBindingResolver(codegenContext.model, ProtocolContentTypes.consistent("application/json"))
+        RestJsonHttpBindingResolver(codegenContext.model, ProtocolContentTypes("application/json", "application/json", "application/vnd.amazon.eventstream"))


I think it does.

crisidev · 2022-07-13T10:41:31Z

...amazon/smithy/rust/codegen/smithy/protocols/serialize/EventStreamErrorMarshallerGenerator.kt

+import software.amazon.smithy.rust.codegen.util.hasTrait
+import software.amazon.smithy.rust.codegen.util.toPascalCase
+
+class EventStreamErrorMarshallerGenerator(


Let's add some documentation to this class and methods..

...ware/amazon/smithy/rust/codegen/smithy/protocols/serialize/EventStreamMarshallerGenerator.kt

crisidev · 2022-07-13T10:44:19Z

rust-runtime/aws-smithy-http-server/examples/pokemon_service/tests/simple_integration_test.rs

@@ -10,15 +10,42 @@
 use std::time::Duration;

 use crate::helpers::{client, PokemonService};
+use async_stream::stream;


Be aware the this file is currently symlinked inside the Python folder. You will need to copy the original implementation there since event streams are not supported yet.

jdisanti

I want to take a look at the codegen diff and CI results before approving, but I think overall this looks good! One question remaining that may or may not require changes.

jdisanti · 2022-07-13T20:45:57Z

...main/kotlin/software/amazon/smithy/rust/codegen/smithy/transformers/EventStreamNormalizer.kt

-            }
+    fun transform(model: Model): Model = ModelTransformer.create().mapShapes(model) { shape ->
+        if (shape is OperationShape && shape.isEventStream(model)) {
+            addStreamErrorsToOperationErrors(model, shape)


Does this affect the top-level error, the operation error, or both? I think the intention is to only have the top-level error hold both the operation and streaming errors in it.

* Server event streams * Rename EventStreamInput to EventStreamSender * Make event stream errors optional * Pokemon service model updated * Pokemon server event handler * Pokemon client to test event streams * EventStreamDecorator to make optional using SigV4 signing Closes: #1157 Signed-off-by: Daniele Ahmed <[email protected]>

Signed-off-by: Daniele Ahmed <[email protected]>

rcoh

Wow! This is an incredible piece of engineering with a lot of cool ideas baked in. Really impressed with how quick this came together.

In the current state of the PR I can't really evaluate the changes, especially the changes around errors for event streams. Here's what I'd suggest:

An RFC / design doc (doesn't need to be long) or an addition to the docs folder explaining different union shape structures and the event stream types we plan to generate
As possible, split out the most complex pieces of code into standalone PRs with tests, eg. the changes to EventStream normalization can stand alone I think?

rust-runtime/aws-smithy-eventstream/src/frame.rs

rcoh · 2022-07-14T17:49:26Z

...amazon/smithy/rust/codegen/smithy/protocols/serialize/EventStreamErrorMarshallerGenerator.kt

+import software.amazon.smithy.rust.codegen.util.hasTrait
+import software.amazon.smithy.rust.codegen.util.toPascalCase
+
+class EventStreamErrorMarshallerGenerator(


does this really need to inherit? can we refactor for composition?

Signed-off-by: Daniele Ahmed <[email protected]>

github-actions · 2022-07-18T13:58:53Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Server Test (ignoring whitespace)
No codegen difference in the Server Test Python

A new doc preview is ready to view.

Signed-off-by: Daniele Ahmed <[email protected]>

github-actions · 2022-07-19T15:08:23Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Server Test (ignoring whitespace)
No codegen difference in the Server Test Python

A new doc preview is ready to view.

Signed-off-by: Daniele Ahmed <[email protected]>

github-actions · 2022-07-19T18:26:41Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Server Test (ignoring whitespace)
No codegen difference in the Server Test Python

A new doc preview is ready to view.

Signed-off-by: Daniele Ahmed <[email protected]>

github-actions · 2022-07-19T19:24:20Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Server Test (ignoring whitespace)
No codegen difference in the Server Test Python

A new doc preview is ready to view.

Signed-off-by: Daniele Ahmed <[email protected]>

github-actions · 2022-07-21T19:19:34Z

A new generated diff is ready to view.

A new doc preview is ready to view.

jdisanti

Very nice work! 🚀

I checked out the changes locally and verified that the Amazon Transcribe streaming example still works correctly. Also looked through the generated documentation to see how the errors look there. Everything looks good to me.

Overall, I'm happy with these changes. The only snag left is that the codegen diff doesn't show a diff for a full service thanks to #1219. I think if we get #1565 merged, and then update this with the latest main, the diff will be small enough that we can see a full service diffed. Seeing that would be a nice sanity check.

I think we're very close to getting this merged. @rcoh or @Velfi, feel free to approve on my behalf if I'm not available to review with the changelog and codegen diff updates in.

CHANGELOG.next.toml

Signed-off-by: Daniele Ahmed <[email protected]>

github-actions · 2022-07-22T13:15:17Z

A new generated diff is ready to view.

A new doc preview is ready to view.

82marbag · 2022-07-25T12:52:24Z

I tried #1565 here, but the diff of errors is still as large (20k lines). Instead, with and without #1565, I generated the whole SDK for both main and eventstreams, sorted all files and created a diff.
This is the diff without #1565. It's not large and it can be reviewed manually; the only changes are related to event streams (mainly additions).
@rcoh suggested to run semver check as well and it's all PASS

This reverts commit 5779a0d.

Signed-off-by: Daniele Ahmed <[email protected]>

github-actions · 2022-07-25T16:41:08Z

A new generated diff is ready to view.

A new doc preview is ready to view.

rcoh · 2022-07-21T16:01:14Z

...otlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerServiceGenerator.kt

@@ -74,7 +74,7 @@ open class ServerServiceGenerator(

    // Render combined errors.
    open fun renderCombinedErrors(writer: RustWriter, operation: OperationShape) {
-        ServerCombinedErrorGenerator(coreCodegenContext.model, coreCodegenContext.symbolProvider, operation).render(writer)
+        /* Subclasses can override */


should we make it abstract?

…degenVisitor` This is a holdover from when the server subproject was started. We've never utilized this model transformer, nor will we have any use for it now, since event stream operations are supported in the server since #1479. See #1762 (comment)

…degenVisitor` (#1764) This is a holdover from when the server subproject was started. We've never utilized this model transformer, nor will we have any use for it now, since event stream operations are supported in the server since #1479. See #1762 (comment).

82marbag requested review from a team as code owners June 21, 2022 17:24

82marbag added the needs-sdk-review label Jun 21, 2022

jdisanti reviewed Jun 21, 2022

View reviewed changes

jdisanti reviewed Jun 22, 2022

View reviewed changes

crisidev mentioned this pull request Jun 28, 2022

Reconcile Python specific pokemon.smithy model #1508

Closed

82marbag force-pushed the eventstreams branch from e6eb8cc to 26ceeea Compare June 30, 2022 11:58

jdisanti reviewed Jul 1, 2022

View reviewed changes

jdisanti reviewed Jul 7, 2022

View reviewed changes

82marbag force-pushed the eventstreams branch from 97333d1 to 0a150ba Compare July 8, 2022 19:24

jdisanti reviewed Jul 12, 2022

View reviewed changes

crisidev approved these changes Jul 13, 2022

View reviewed changes

jdisanti reviewed Jul 13, 2022

View reviewed changes

82marbag force-pushed the eventstreams branch from 24af240 to 7ec3c9c Compare July 14, 2022 09:04

82marbag added 4 commits July 14, 2022 11:28

fix function signature

49e4cc5

Signed-off-by: Daniele Ahmed <[email protected]>

revert tests for python

cd3fa0d

Signed-off-by: Daniele Ahmed <[email protected]>

update event stream marshalling tests

9c60845

remove todo on pokemon server

8f7e03f

rcoh requested changes Jul 14, 2022

View reviewed changes

82marbag added 8 commits July 15, 2022 10:04

document sign_empty None

54ffd3f

Signed-off-by: Daniele Ahmed <[email protected]>

add documentation and rfc

a96ce2c

Signed-off-by: Daniele Ahmed <[email protected]>

update tests

dd1856b

Signed-off-by: Daniele Ahmed <[email protected]>

update tests

dcc0dac

Signed-off-by: Daniele Ahmed <[email protected]>

address comments

16f6fd9

Signed-off-by: Daniele Ahmed <[email protected]>

update tests

09e9f31

Signed-off-by: Daniele Ahmed <[email protected]>

render errors once

2c700a5

Signed-off-by: Daniele Ahmed <[email protected]>

fix PythonCodegenServerPlugin

0ff3c5d

update tests

f1c932f

Signed-off-by: Daniele Ahmed <[email protected]>

Daniele Ahmed and others added 2 commits July 19, 2022 19:27

address comments

011e02a

Signed-off-by: Daniele Ahmed <[email protected]>

render from errors once

193ae52

Signed-off-by: Daniele Ahmed <[email protected]>

remove unused import in test

6f77fb5

Signed-off-by: Daniele Ahmed <[email protected]>

82marbag added 5 commits July 21, 2022 17:59

refactor errors, generate on demand

ecc60f4

Signed-off-by: Daniele Ahmed <[email protected]>

Merge branch 'main' into eventstreams

6173c6e

Merge branch 'main' into eventstreams

5a10f87

update CHANGELOG

0fce671

Signed-off-by: Daniele Ahmed <[email protected]>

update TopLevelErrorGenerator

4f51dd4

Signed-off-by: Daniele Ahmed <[email protected]>

jdisanti reviewed Jul 21, 2022

View reviewed changes

CHANGELOG.next.toml Outdated Show resolved Hide resolved

CHANGELOG.next.toml Outdated Show resolved Hide resolved

82marbag and others added 2 commits July 22, 2022 15:01

update CHANGELOG

b446403

Signed-off-by: Daniele Ahmed <[email protected]>

Sort inline dependencies alphabetically by key

5779a0d

Daniele Ahmed and others added 2 commits July 25, 2022 18:21

Revert "Sort inline dependencies alphabetically by key"

f7fe78d

This reverts commit 5779a0d.

Merge branch 'main' into eventstreams

8f6ba8c

jdisanti approved these changes Jul 25, 2022

View reviewed changes

rename rfc file

f389a45

Signed-off-by: Daniele Ahmed <[email protected]>

82marbag enabled auto-merge (squash) July 25, 2022 17:13

rcoh approved these changes Jul 25, 2022

View reviewed changes

82marbag merged commit 3610085 into main Jul 25, 2022

82marbag deleted the eventstreams branch July 25, 2022 17:24

david-perez mentioned this pull request Sep 23, 2022

Remove RemoveEventStreamOperations model transformer from ServerCodegenVisitor #1764

Merged

Velfi removed the needs-sdk-review label Dec 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support server event streams #1479

Support server event streams #1479

82marbag commented Jun 21, 2022

jdisanti left a comment

jdisanti left a comment

jdisanti Jul 12, 2022

crisidev left a comment

crisidev Jul 13, 2022

crisidev Jul 13, 2022

crisidev Jul 13, 2022

jdisanti left a comment

jdisanti Jul 13, 2022

rcoh left a comment

rcoh Jul 14, 2022

github-actions bot commented Jul 18, 2022

github-actions bot commented Jul 19, 2022

github-actions bot commented Jul 19, 2022

github-actions bot commented Jul 19, 2022

github-actions bot commented Jul 21, 2022

jdisanti left a comment

github-actions bot commented Jul 22, 2022

82marbag commented Jul 25, 2022

github-actions bot commented Jul 25, 2022

rcoh Jul 21, 2022

		RestJsonHttpBindingResolver(codegenContext.model, ProtocolContentTypes.consistent("application/json"))
		RestJsonHttpBindingResolver(codegenContext.model, ProtocolContentTypes("application/json", "application/json", "application/vnd.amazon.eventstream"))

Support server event streams #1479

Support server event streams #1479

Conversation

82marbag commented Jun 21, 2022

Motivation and Context

Description

Testing

Checklist

jdisanti left a comment

Choose a reason for hiding this comment

jdisanti left a comment

Choose a reason for hiding this comment

jdisanti Jul 12, 2022

Choose a reason for hiding this comment

crisidev left a comment

Choose a reason for hiding this comment

crisidev Jul 13, 2022

Choose a reason for hiding this comment

crisidev Jul 13, 2022

Choose a reason for hiding this comment

crisidev Jul 13, 2022

Choose a reason for hiding this comment

jdisanti left a comment

Choose a reason for hiding this comment

jdisanti Jul 13, 2022

Choose a reason for hiding this comment

rcoh left a comment

Choose a reason for hiding this comment

rcoh Jul 14, 2022

Choose a reason for hiding this comment

github-actions bot commented Jul 18, 2022

github-actions bot commented Jul 19, 2022

github-actions bot commented Jul 19, 2022

github-actions bot commented Jul 19, 2022

github-actions bot commented Jul 21, 2022

jdisanti left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 22, 2022

82marbag commented Jul 25, 2022

github-actions bot commented Jul 25, 2022

rcoh Jul 21, 2022

Choose a reason for hiding this comment